Skip to content

Conversation

@rbanka1
Copy link
Contributor

@rbanka1 rbanka1 commented Jan 8, 2025

based on https://github.com/pmem/pmemstream/tree/master/utils/check_license

Description

Added license checker and exception file.

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used

Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the second commit, with just a permission update should be squashed.

Also, at best the first commit would contain orignal script, and the second commit would contain changes for this repo (incl. dates update)

@lukaszstolarczuk lukaszstolarczuk marked this pull request as ready for review January 9, 2025 10:26
@lukaszstolarczuk lukaszstolarczuk requested a review from a team as a code owner January 9, 2025 10:26
@PatKamin
Copy link
Contributor

PatKamin commented Jan 9, 2025

There is an error in both new jobs: grep: write error: Broken pipe

@lukaszstolarczuk lukaszstolarczuk marked this pull request as draft January 9, 2025 10:47
@lukaszstolarczuk lukaszstolarczuk mentioned this pull request Jan 9, 2025
10 tasks
@lukaszstolarczuk lukaszstolarczuk marked this pull request as ready for review January 9, 2025 12:21
@ldorau
Copy link
Contributor

ldorau commented Jan 9, 2025

There is an error in both new jobs: grep: write error: Broken pipe

I think we can ignore it. I cannot see any reason of that.

@PatKamin
Copy link
Contributor

PatKamin commented Jan 9, 2025

There is an error in both new jobs: grep: write error: Broken pipe

I think we can ignore it. I cannot see any reason of that.

Me neither. CI works as supposed, locally I cannot reproduce - LGTM

CURRENT_COMMIT=$($GIT --no-pager log --pretty=%H -1)
MERGE_BASE=$($GIT merge-base HEAD origin/main 2>/dev/null)
[ -z $MERGE_BASE ] && \
MERGE_BASE=$($GIT --no-pager log --pretty="%cN:%H" | grep GitHub | head -n1 | cut -d: -f2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this fallback causes error because of SIGPIPE signal. Redirecting stderr solves the problem (like in 72 line)
--pretty="%cN:%H" 2>/dev/null | grep "GitHub" 2>/dev/null | head -n 1 | cut -d: -f2)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your help, your idea solved the problem 🥇

Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 3 commits:
https://github.com/oneapi-src/unified-memory-framework/pull/1028/commits
should be squashed into one (2nd and 3rd commit are fixes of the 1st one)

@lukaszstolarczuk
Copy link
Contributor

lukaszstolarczuk commented Jan 10, 2025

All 3 commits: https://github.com/oneapi-src/unified-memory-framework/pull/1028/commits should be squashed into one (2nd and 3rd commit are fixes of the 1st one)

@ldorau, I discussed this with @rbanka1, I believe it's ok. The first one (more or less) introduces new script (based on pmemstream), the second one extends its capabilities, and the third one fixes the issue, which existed also in the original script... do you think it's not ok to have all these 3 commits here...?

@ldorau
Copy link
Contributor

ldorau commented Jan 10, 2025

All 3 commits: https://github.com/oneapi-src/unified-memory-framework/pull/1028/commits should be squashed into one (2nd and 3rd commit are fixes of the 1st one)

@ldorau, I discussed this with @rbanka1, I believe it's ok. The first one (more or less) introduces new script (based on pmemstream), the second one extends its capabilities, and the third one fixes the issue, which existed also in the original script... do you think it's not ok to have all these 3 commits here...?

@lukaszstolarczuk So why not squash commit 3rd with 2n?

@lukaszstolarczuk
Copy link
Contributor

All 3 commits: https://github.com/oneapi-src/unified-memory-framework/pull/1028/commits should be squashed into one (2nd and 3rd commit are fixes of the 1st one)

@ldorau, I discussed this with @rbanka1, I believe it's ok. The first one (more or less) introduces new script (based on pmemstream), the second one extends its capabilities, and the third one fixes the issue, which existed also in the original script... do you think it's not ok to have all these 3 commits here...?

@lukaszstolarczuk So why not squash commit 3rd with 2n?

we can do that :) - please remember @rbanka1 to update commit msg with all changes

@rbanka1 rbanka1 force-pushed the licenseNew branch 2 times, most recently from d8550c5 to 007a4cd Compare January 13, 2025 10:11
based on https://github.com/pmem/pmemstream/tree/master/utils/check_license
Changes for this repository:
- update dates
- file exceptions
… fixes

Changes for this commit:
- adding a condition to check the validity of the starting date
- broken pipe fix
- updating date fix
@lukaszstolarczuk lukaszstolarczuk merged commit bf8d0d8 into oneapi-src:main Jan 14, 2025
78 checks passed
@rbanka1 rbanka1 deleted the licenseNew branch February 18, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants